The conditional logic at line 1407 checks isMessageDelivered, but the message could be read (which implies delivered). The condition should check for both read and delivered states to ensure MessageReadStatusIcon is always called with the isMessageDelivered parameter, maintaining consistency. Consider simplifying this to always pass the isMessageDelivered parameter.

This is done to prevent breaking changes in the component factory.
If isMessageDelivered == true, that means we can call the new overloaded function; otherwise, we have to call the deprecated one.

Just wondering if need to expose these new methods as public - do you think they would be useful for integrators, without the risk of us having to change the implementation/Public API?

I would say so. But I'm fine with making it internal and exposing it on demand.
I will change it then.

I don't really have a very strong opinion here - if you think this would be a stable API, we can leave it exposed.

I would use the non-throwing method here, like getCreatedAtOrDefault()

Do you think it would be possible to avoid introducing new Plugins, as it is something that we want to completely remove in the future. I think that this introduces another layer of complexity over the ChatClient. Wouldn't it be possible to achieve the same thing by calling the appropriate MessageReceiptManager methods from the corresponding ChatClient methods (queryChannels/queryChannel)?

I wouldn’t say the long-term goal is to remove plugins entirely, but rather to make them internal and non-optional.
In this case, regardless of the approach, the implementation would likely live outside ChatClient — which is already close to 5K LOC.
Keeping it as an internal plugin for now helps isolate responsibilities and prevents adding more logic directly to ChatClient.
We can revisit and consolidate it later as part of the broader plugin refactoring.

I think that we are reducing not more than 3 lines at best in the client by introducing the Plugin - two invocations of the MessageReceiptManager. I am generally against the introduction of new Plugins, because they are executing crucial business logic as a side-effect, and it's really difficult to locate that logic, unless you explicitly know about the existence of that specific Plugin.

But nevertheless, let's keep it like this, but I definitely agree that should revisit this as part of the planned refactor.

Would this work for the case when push notifications are received, if the app is killed? In that case, we wouldn't have a connected user to check against.

Can we somehow avoid the dependency on the OfflinePlugin RepositoryFacade?
To be honest I am mostly concerned about the case where the OfflinePlugin is not present, and we have to make calls to getMessage/getChannel, which if happens often enough, we could end up in a rate limit error. (Note the case where a customer doesn't have the OfflinePlugin, and is in a channel where multiple message.new events are received in short amount of time - we could end up firing getChannel lots of times)

Can we somehow remove the need for the retrieveChannel/retrieveMessage methods in the manager?
I am thinking in the direction of:

• Instead of depending on the OfflinePlugin/HTTP calls, why don't we store some channel config in the client database after fetching the channel(s). Later, when we call markMessageAsDelivered, we can check that stored config, to make sure we should mark the message.

This whole logic should be easier when the state/db is moved down to the client...

I see. In that case, we need to store more than just the channel config. We use message data and channel read, too.
I will include Message, Channel, ChannelUserRead, and ChannelConfig in the client database then.
Expects an even bigger PR 😅

Hmm but my question still stands: Isn't hitting rate limit errors a real possibility?

Side note: We can also access some of the State plugin data from Client via:

logicRegistry
            ?.channelStateLogic("type", "id")
            ?.listenForChannelState()
            ?.toChannel() // ?.getMessageById(id)

Perhaps this can be utilised to check for existing channel data (but this still wouldn't cover the case when the client is used without the state plugin)

The channel state is not completely filled when the app is not in memory (for example, the read list is empty). So we can't rely on it.
We can make the delivery receipts feature require the offline plugin, so we can rely on the local storage instead (local storage will be available soon as a built-in SDK feature).

Is the type needed? As far as I can see, if is always set to delivery.

The goal is to support other types of message receipt, like read, in the future.
The current read implementation is quite confusing and hard to maintain 😓

I am just afraid that this will become legacy code, if we don't actually update this to support the different receipts.

Let's remove it then

I am not sure whether we really need this class - wouldn't it just introduce another layer of complexity?

The goal is to avoid having many constructor parameters when a class depends on multiple repositories.
For example, when the manager depends on MessageRepository, ChannelRepository, and MessageReceiptRepository, we would have to pass 3 dependencies instead of 1

You should probably want to also check for lastRead and promote it to delivered for supporting old messages where lastDeliveredAt is not present.

The Channel.deliveredReadsOf function returns a list of ChannelUserRead objects representing which ones have delivered the given message, regardless of the read state. If lastDeliveredAt is null, as in old messages, it is not returned in the list.
For reads, we can use Channel.readsOf function, which checks specifically read states, regardless of the delivered states.
That's to give customers the flexibility to list who has read messages and who has delivered messages, exclusively.
If they want to list delivered reads that are not read, they can do:
channel.deliveredReadsOf(message) - channel.readsOf(message)

Should we also have a corresponding stop() method? Or do we rely on cancelling the scope to cancel the running job?

I believe that the default PushNotificationReceivedListener is no longer needed.

I am not sure whether this should also control the default showing of push notifications, because it is already configurable by extending the NotificationHandler. Should this maybe just be a side effect?

Also I believe that this method will be called for any PN, not just for message.new, so for the delivery receipt case, the integrators would also need to check the type of the message.

Do we need the .db suffix in the name of the DB?

Also another thing that I missed initially: Should we have different DB files per user (similar to the OfflinePlugin DB)?

Small point for the future, no need to change anything now: We could potentially unify the common converters with the ones from the offline plugin.

As I mentioned in a previous comment, I believe that this will be called for every PN type -> shouldn't we do this just for message.new?

Should we maybe return Result.Error from this method, if the message cannot be marked as delivered (ex. if some of the preconditions are not satisfied)?

I think it would be good to document that this method might attempt to internally call getMessage/getChannel, just for transparency sake. Or at least it would be good to document this in the Docs page as well. I think that it is an important information, that integrators should know before using this method.